Skip to content

Conversation

@Snevzor
Copy link
Contributor

@Snevzor Snevzor commented May 29, 2024

Certain display drivers allow for setting an initial display orientation using a device tree property. LVGL should take this into account while initializing.

Certain display drivers allow for setting an initial display orientation
using a device tree property. LVGL should take this into account while
initializing.

I could have written "disp_drv.rotated = disp_data.cap.current_orientation"
because the enums match, but they are different types and they might get
mismatched in the future.

Signed-off-by: Sven Depoorter <[email protected]>
@zephyrbot zephyrbot added the area: LVGL Light and Versatile Graphics Library Support label May 29, 2024
@zephyrbot zephyrbot requested review from brgl, faxe1008 and pdgendt May 29, 2024 14:35
@faxe1008
Copy link
Contributor

Did you check by any chance how the rotated flag changes behaviour of the input devices when they are reporting touch coordinates?

@Snevzor
Copy link
Contributor Author

Snevzor commented May 29, 2024

@faxe1008 I didn't think of this when submitting.

I don't have such input device to test with so I tried to build for SDL. Unfortunately, I have dependency issues to install the correct SDL version which I can't quickly resolve, so no "quick testing" for now.

#73300 seems to take care but I'll have to check further if indeed the .rotated flag messes this up now somewhere in LVGL.

@faxe1008
Copy link
Contributor

faxe1008 commented May 29, 2024

I patched rotation support quite crudely into the sdl display:

diff --git a/drivers/display/display_sdl.c b/drivers/display/display_sdl.c
index e929a775f9b..601e9f20c22 100644
--- a/drivers/display/display_sdl.c
+++ b/drivers/display/display_sdl.c
@@ -35,6 +35,7 @@ struct sdl_display_data {
 	void *read_texture;
 	bool display_on;
 	enum display_pixel_format current_pixel_format;
+	enum display_orientation current_orientation;
 	uint8_t *buf;
 	uint8_t *read_buf;
 };
@@ -260,9 +261,15 @@ static int sdl_display_write(const struct device *dev, const uint16_t x,
 		sdl_display_write_bgr565(disp_data->buf, desc, buf);
 	}
 
-	sdl_display_write_bottom(desc->height, desc->width, x, y,
+	if(disp_data->current_orientation == DISPLAY_ORIENTATION_NORMAL) {
+		sdl_display_write_bottom(desc->height, desc->width, x, y,
 				 disp_data->renderer, disp_data->mutex, disp_data->texture,
-				 disp_data->buf, disp_data->display_on);
+				 disp_data->buf, disp_data->display_on, 0);
+	} else if(disp_data->current_orientation == DISPLAY_ORIENTATION_ROTATED_180) {
+		sdl_display_write_bottom(desc->height, desc->width, x, y,
+				 disp_data->renderer, disp_data->mutex, disp_data->texture,
+				 disp_data->buf, disp_data->display_on, 180);
+	}
 
 	return 0;
 }
@@ -516,6 +523,7 @@ static const struct display_driver_api sdl_display_api = {
 	static struct sdl_display_data sdl_data_##n = {			\
 		.buf = sdl_buf_##n,					\
 		.read_buf = sdl_read_buf_##n,				\
+		.current_orientation = DISPLAY_ORIENTATION_ROTATED_180,\
 	};								\
 									\
 	DEVICE_DT_INST_DEFINE(n, &sdl_display_init, NULL,		\
diff --git a/drivers/display/display_sdl_bottom.c b/drivers/display/display_sdl_bottom.c
index 0e995a341c9..b8e0139334a 100644
--- a/drivers/display/display_sdl_bottom.c
+++ b/drivers/display/display_sdl_bottom.c
@@ -67,7 +67,7 @@ int sdl_display_init_bottom(uint16_t height, uint16_t width, uint16_t zoom_pct,
 void sdl_display_write_bottom(const uint16_t height, const uint16_t width,
 			      const uint16_t x, const uint16_t y,
 			      void *renderer, void *mutex, void *texture,
-			      uint8_t *buf, bool display_on)
+			      uint8_t *buf, bool display_on, double angle)
 {
 	SDL_Rect rect;
 	int err;
@@ -87,7 +87,7 @@ void sdl_display_write_bottom(const uint16_t height, const uint16_t width,
 
 	if (display_on) {
 		SDL_RenderClear(renderer);
-		SDL_RenderCopy(renderer, texture, NULL, NULL);
+		SDL_RenderCopyEx(renderer, texture, NULL, NULL, angle, NULL, SDL_FLIP_NONE);
 		SDL_RenderPresent(renderer);
 	}
 
diff --git a/drivers/display/display_sdl_bottom.h b/drivers/display/display_sdl_bottom.h
index 54973777f1b..1d86b551838 100644
--- a/drivers/display/display_sdl_bottom.h
+++ b/drivers/display/display_sdl_bottom.h
@@ -26,7 +26,7 @@ int sdl_display_init_bottom(uint16_t height, uint16_t width, uint16_t zoom_pct,
 void sdl_display_write_bottom(const uint16_t height, const uint16_t width,
 			      const uint16_t x, const uint16_t y,
 			      void *renderer, void *mutex, void *texture,
-			      uint8_t *buf, bool display_on);
+			      uint8_t *buf, bool display_on, double angle);
 int sdl_display_read_bottom(const uint16_t height, const uint16_t width,
 			    const uint16_t x, const uint16_t y,
 			    void *renderer, void *buf, uint16_t pitch,

rotated.mp4

Yeah this is definetely an issue.

@Snevzor
Copy link
Contributor Author

Snevzor commented May 29, 2024

If lvgl_pointer_process_event has a similar approach as in https://github.com/lvgl/lvgl/blob/master/src/indev/lv_indev.c#L636

I did not expect this to be an issue but your test clearly shows there is an issue.

@faxe1008
Copy link
Contributor

faxe1008 commented May 29, 2024

From the looks of it LVGL should be doing the correct thing, I need to test this on real hardware though not on the bodged native_sim. Since I am out of office for the rest of the week I won't able to though.

EDIT:// Yeah forgot to add the rotation into the capabilites struct within my hack:

@@ -455,6 +462,7 @@ static void sdl_display_get_capabilities(
        struct sdl_display_data *disp_data = dev->data;

        memset(capabilities, 0, sizeof(struct display_capabilities));
+       capabilities->current_orientation = disp_data->current_orientation;

With LVGL performing the rotation this code needs to be removed since it will apply the rotation a second time:

/* rotate touch point to match display rotation */

Can you maybe remove this within your PR and add an explanation for it?

@Snevzor
Copy link
Contributor Author

Snevzor commented May 30, 2024

I would like to do that, but I'm not sure if I'm able to because I have no means to test my changes.

It also looks like a large part of #73300 needs to be reverted (or completely?) because it introduced tmp_point as well.

@faxe1008
Copy link
Contributor

Well you can verify the changes with a unit test:

  • setup the native sim and remove the pointer to the display controller dev handle so you can also generate events from the test
  • send the input events using the input api simulating the display press
  • get the point from LVGL using lv_indev_get_point

With this setup you can test the rotations across the entire code path.

@Snevzor
Copy link
Contributor Author

Snevzor commented May 30, 2024

Sounds like a good idea @faxe1008. Unfortunately I still can't install SDL2 (32bit version) on my system without breaking lots of stuff. I also can't seem to find a docker image for this purpose (with display/sdl suport).
I'm new to ZTEST but perhaps I don't need SDL2 at all for this with the display_dummy driver?
I would also have to patch the display_dummy driver with orientation setting and without #73360 approved I still don't know if the way rotation is handled in the driver is correct.
I have limited time left before my holiday though.

@faxe1008
Copy link
Contributor

@Snevzor there is also the native_sim64 target which should allow you to use the 64-bit SDL library.
Anyways I have started working on the tests: https://github.com/zephyrproject-rtos/zephyr/compare/main...faxe1008:zephyr:lvgl_input_tests?expand=1, will submit a PR once ready.

@Snevzor
Copy link
Contributor Author

Snevzor commented May 30, 2024

@faxe1008 thanks for the native_sim64 hint!

The test you are writing will definitely fill a gap, and it looks great already! I'm learning a lot from it.

@faxe1008
Copy link
Contributor

@Snevzor I have opened #73556 so others will also be able to test the hardware rotation. To cut down on a lot of boilerplate it would make sense for the testing PR to add something similar to what you have done in this PR otherwise there will be an explosion of overlay files in the test.

@Snevzor
Copy link
Contributor Author

Snevzor commented May 31, 2024

Yes that makes sense @faxe1008. This PR can be closed then ;)

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 31, 2024
@github-actions github-actions bot closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: LVGL Light and Versatile Graphics Library Support Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants